Skip to content

fix(send): correct group phash and mark full SKDM target set (WA Web parity) - #678

Merged
jlucaso1 merged 2 commits into
mainfrom
fix/group-skdm-phash-base64-and-haskey
Jun 1, 2026
Merged

fix(send): correct group phash and mark full SKDM target set (WA Web parity)#678
jlucaso1 merged 2 commits into
mainfrom
fix/group-skdm-phash-base64-and-haskey

Conversation

@jlucaso1

@jlucaso1 jlucaso1 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

Three related correctness fixes on the group SKDM/send path. The phash ones are validated against a real WhatsApp Web capture sent to the production server (raw LIDs redacted as PII; reproduced locally to confirm the formula).

1. phash base64: standard alphabet

participant_list_hash used URL-safe base64 (-/_). whatsmeow (RawStdEncoding) and WA Web (WABase64.encodeB64) use the standard alphabet (+//). Any phash whose first 6 hash bytes hit base64 index 62/63 diverged from the server (~22% of phashes). One-line fix plus a cross-impl test vector.

2. has_key marks the full SKDM target set

After a group send we marked only the devices that encrypted successfully (skdm_encrypted_devices). WA Web's markHasSenderKey(x, M) marks the whole target set M. A device that fails to encrypt (406 / no bundle) used to stay has_key=false, so it was re-targeted on every send, re-fetching dead prekeys, which is a big part of the cold-send retry storm. PreparedGroupStanza.skdm_devices now carries the full distribution_list. The retry-receipt path (mark_forget_sender_key) still repairs any device that is actually alive and missing the key.

3. phash over the full device set + self, on every send

WA Web computes phashV2([].concat(A, [B])) where A is the full participant device set and B is the sending device, and it sends a phash on every group send. We were:

  • hashing only the SKDM target subset and omitting the sending device;
  • omitting the phash entirely on fully-warm sends (no distribution).

resolve_skdm_targets now returns (all_devices, needs_skdm); prepare_group_stanza takes all_devices_for_phash and computes the phash via a new build_group_phash_set helper (full set + sending device, hosted-filtered) regardless of whether any SKDM is distributed. Status broadcasts keep their prior behavior (this is group-only; WA Web's status path does not augment the set with self).

Validation (real WA Web capture, prod server)

  • Cold send: the recipient <to> device set plus the sending device reproduced the exact phash on the wire; the recipient set alone produced a different hash, so the sending device is part of the hashed set.
  • Two later warm sends (no <participants> node) still carried a phash, confirming a phash is sent on every group send.
  • The <ack class="message"> never echoes a phash, so the server validates it silently; a correct phash is what keeps it from triggering a re-fanout.

Tests

  • participant_list_hash standard-base64 cross-impl vectors (whatsmeow/WA Web parity).
  • build_group_phash_set: self inclusion changes the hash; hosted devices are dropped (fictitious vectors, no PII).
  • prepare_group_stanza: a device whose SKDM encryption fails is still marked has_key (the full target set).

cargo clippy --all-targets -- -D warnings clean; wacore + whatsapp-rust + sqlite-storage test suites green.

…parity)

Three related group-SKDM correctness fixes, the phash ones validated against a
real WA Web capture sent to the production server (raw identifiers redacted as
PII; reproduced locally to confirm the formula).

1. phash base64: use standard alphabet ('+'/'/', `BASE64_STANDARD_NO_PAD`) to
   match whatsmeow (`RawStdEncoding`) and WA Web (`WABase64.encodeB64`). URL-safe
   ('-'/'_') diverged on ~22% of phashes (any output hitting base64 index 62/63).

2. has_key marks the FULL SKDM target set: `PreparedGroupStanza.skdm_devices`
   now carries the whole `distribution_list`, not just the devices that
   encrypted successfully. Mirrors WA Web `markHasSenderKey(x, M)`. Previously
   devices that failed to encrypt (406 / no bundle) stayed `has_key=false` and
   were re-targeted on every send, re-fetching dead prekeys -> retry storm. The
   retry-receipt path (`mark_forget_sender_key`) repairs any that are actually
   alive and keyless.

3. phash over the full participant device set + the sending device, on EVERY
   group send (matching WA Web `phashV2([].concat(A, [B]))`):
   - was computed over the SKDM target subset only and omitted the self device;
   - was omitted entirely on fully-warm sends (no distribution).
   `resolve_skdm_targets` now returns `(all_devices, needs_skdm)` so the caller
   can pass the full set; `prepare_group_stanza` gains `all_devices_for_phash`
   and computes the phash via `build_group_phash_set` (full set + self,
   hosted-filtered) regardless of whether any SKDM is distributed. Status
   broadcasts keep their prior behavior (group-only change).

Validation against a real WA Web capture (prod server):
- cold send: the recipient `<to>` set plus the sending device reproduced the
  exact on-wire phash; the recipient set alone produced a different hash, so the
  sending device is part of the hashed set.
- two later warm sends (no `<participants>`) still carried a phash, confirming a
  phash is sent on every group send.
- the `<ack class="message">` never echoes a phash, so the server validates it
  silently; a correct phash is what keeps it from triggering a re-fanout.

Tests: a phash device-set test (self inclusion + hosted filtering, fictitious
vectors), and a prepare_group_stanza test proving a device whose SKDM
encryption fails is still marked has_key.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 400cd76b-a4d3-45c5-8c9c-44bbe060eadd

📥 Commits

Reviewing files that changed from the base of the PR and between 4383cb7 and 0ca1264.

📒 Files selected for processing (1)
  • wacore/benches/send_receive_benchmark.rs

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Participant hash now uses standard Base64 for compatibility with WhatsApp Web/server.
  • Improvements
    • Refined group and status delivery targeting so sender-key distribution and group phash computation match WA Web semantics, reducing redundant re-targeting.
    • Group sends now include the full intended SKDM target set when marking devices, even if some encryptions fail.
  • Tests
    • Added/regressed tests validating phash vectors and SKDM marking behavior.

Walkthrough

Split resolved device targets into (all_devices_for_phash, needs_skdm); propagate both through status and group send paths, update prepare_group_stanza signature and phash computation (standard Base64), and adjust stanza SKDM marking and tests/benchmarks.

Changes

SKDM Targeting and Phash Computation Refactor

Layer / File(s) Summary
Phash base64 encoding fix
wacore/src/messages.rs
participant_list_hash now uses standard Base64 (+//) and adds phash_crosscheck_vectors golden tests validating exact phash outputs.
SKDM target resolution split
src/send.rs
resolve_skdm_targets now returns Option<(Vec<Jid>, Vec<Jid>)> = (all_devices, needs_skdm); computes needs_skdm from cached sender-key device map and documents the new return shape.
Status message SKDM targeting
src/send.rs
send_status_message extracts needs_skdm from the resolver tuple and passes None for all_devices_for_phash to preserve prior phash behavior for status broadcasts.
Group message SKDM targeting
src/send.rs
send_message_impl destructures resolver tuple into all_devices_for_phash and skdm_target_devices; warm/partial sends pass all_devices_for_phash to prepare_group_stanza, cold/force-SKDM retries pass None.
Prepare group stanza signature and phash computation
wacore/src/send.rs
prepare_group_stanza signature extended with all_devices_for_phash: Option<Vec<Jid>>; phash for group sends computed from build_group_phash_set(all_devices_for_phash or distribution_list), excluding hosted devices and including sender device with dedup/sort.
SKDM device marking semantics
wacore/src/send.rs
PreparedGroupStanza.skdm_devices now holds the full SKDM distribution list (including devices that failed SKDM encryption) to match WA Web markHasSenderKey semantics.
Test coverage and benchmark updates
wacore/src/send.rs, wacore/benches/send_receive_benchmark.rs
Added tests for full distribution marking on SKDM failure and group phash golden vectors; benchmark call-sites updated to new prepare_group_stanza arg order and to pass all_devices_for_phash for warm sends.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested labels

api-design

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: fixing group phash computation and SKDM target set marking for WA Web parity, which are the primary objectives of the changeset.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the three correctness fixes, validation methodology, and test coverage for the group SKDM/send path improvements.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/group-skdm-phash-base64-and-haskey

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown

Benchmark Results

4 regression(s) detected (>2% threshold):

Benchmark Current Baseline Change
send_receive_benchmark::group_send::bench_group_send group_256:setup_group_send_256() 2,733,002 1,761,035 +55.2%
send_receive_benchmark::group_send::bench_group_send group_50:setup_group_send_50() 1,212,299 1,027,139 +18.0%
send_receive_benchmark::group_send::bench_group_send group_10:setup_group_send_10() 932,977 888,737 +5.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_256:setup_group_skdm_256() 7,632,524 7,315,573 +4.3%
63 unchanged benchmark(s)
Benchmark Current Baseline Change
reporting_token_benchmark::content_extraction_group::bench_content_extraction simple:setup_simple_message() 3,933 3,933 +0.0%
reporting_token_benchmark::content_extraction_group::bench_content_extraction extended:setup_extended_message() 12,038 12,038 +0.0%
reporting_token_benchmark::key_derivation_group::bench_key_derivation 43,514 43,514 +0.0%
reporting_token_benchmark::token_calculation_group::bench_token_calculation 19,365 19,365 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation simple:setup_full_gen_simple() 68,579 68,579 +0.0%
reporting_token_benchmark::full_generation_group::bench_full_token_generation extended:setup_full_gen_extended() 76,679 76,679 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding simple:setup_simple_message() 2,230 2,230 +0.0%
reporting_token_benchmark::message_encoding_group::bench_message_encoding extended:setup_extended_message() 5,988 5,988 +0.0%
send_receive_benchmark::dm_send::bench_dm_send text:setup_dm_send() 182,806 182,967 -0.1%
send_receive_benchmark::dm_recv::bench_dm_recv text:setup_dm_recv() 2,204,889 2,204,889 +0.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_10:setup_group_skdm_10() 1,142,612 1,131,108 +1.0%
send_receive_benchmark::group_send_skdm::bench_group_send_skdm skdm_50:setup_group_skdm_50() 2,180,053 2,139,072 +1.9%
send_receive_benchmark::group_recv::bench_group_recv text:setup_group_recv() 12,537,260 12,770,526 -1.8%
binary_benchmark::marshal_group::bench_marshal_allocating 71,296 71,296 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_allocating 71,349 71,349 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_allocating 98,416 98,416 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer 78,796 78,796 +0.0%
binary_benchmark::marshal_group::bench_marshal_reusing_buffer_vec_writer 71,396 71,396 +0.0%
binary_benchmark::marshal_group::bench_marshal_long_string 7,599 7,599 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_long_string 7,642 7,642 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_long_string 9,354 9,354 +0.0%
binary_benchmark::marshal_group::bench_marshal_huge_bytes_allocating 530,581 530,581 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_huge_bytes_allocating 530,149 530,149 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_huge_bytes_allocating 531,504 531,504 +0.0%
binary_benchmark::marshal_group::bench_marshal_many_children_allocating 8,506,104 8,506,104 +0.0%
binary_benchmark::marshal_group::bench_marshal_auto_many_children_allocating 8,450,452 8,450,452 +0.0%
binary_benchmark::marshal_group::bench_marshal_exact_many_children_allocating 19,677,941 19,677,941 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal small:setup_small_marshaled() 2,468 2,468 +0.0%
binary_benchmark::unmarshal_group::bench_unmarshal large:setup_large_marshaled() 33,558 33,558 +0.0%
binary_benchmark::unpack_group::bench_unpack_uncompressed 787 787 +0.0%
binary_benchmark::unpack_group::bench_unpack_compressed 526,830 526,830 +0.0%
binary_benchmark::attr_parser_group::bench_attr_parser attr_lookup:setup_attr_marshaled() 4,990 4,990 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip small:setup_small_marshaled() 5,315 5,315 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip large:setup_large_marshaled() 61,874 61,874 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto small:setup_small_marshaled() 5,347 5,347 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_auto large:setup_large_marshaled() 61,942 61,942 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact small:setup_small_marshaled() 6,734 6,734 +0.0%
binary_benchmark::roundtrip_group::bench_roundtrip_exact large:setup_large_marshaled() 85,585 85,585 +0.0%
binary_benchmark::child_iteration_group::bench_get_children_by_tag 477,570 477,570 +0.0%
binary_benchmark::jid_optimization_group::bench_jid_to_owned_access jid_access:setup_jid_heavy_marshaled() 11,563 11,563 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_u32 396 396 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_u32 120 120 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_u64 439 439 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_u64 153 153 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_i64 499 499 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_i64 162 162 +0.0%
numeric_attr_benchmark::bench_group::bench_baseline_loop_100_u64 44,624 44,624 +0.0%
numeric_attr_benchmark::bench_group::bench_proposed_loop_100_u64 16,424 16,424 +0.0%
libsignal_benchmark::dm_group::bench_dm_session_establishment setup:setup_dm_users() 17,225,222 17,372,577 -0.8%
libsignal_benchmark::dm_group::bench_dm_encrypt_first_message first_msg:setup_dm_session() 157,179 157,179 +0.0%
libsignal_benchmark::dm_group::bench_dm_decrypt_first_message decrypt_prekey:setup_dm_with_first_message() 5,513,975 5,513,975 +0.0%
libsignal_benchmark::dm_group::bench_dm_encrypt_subsequent_message subsequent:setup_established_dm_session() 157,539 157,539 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_create_distribution_message create:setup_group_sender() 296,767 296,767 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_encrypt_message encrypt:setup_group_with_distribution() 706,282 706,282 +0.0%
libsignal_benchmark::group_messaging_group::bench_group_decrypt_message decrypt:setup_group_with_encrypted_message() 12,572,707 12,651,392 -0.6%
libsignal_benchmark::conversation_group::bench_full_dm_conversation full:setup_conversation_data() 27,685,808 27,599,080 +0.3%
libsignal_benchmark::signature_group::bench_signature_creation sign:setup_keypair_with_message() 3,467,011 3,467,011 +0.0%
libsignal_benchmark::signature_group::bench_signature_verification verify:setup_keypair_with_message() 126,232,803 126,165,953 +0.1%
libsignal_benchmark::signature_group::bench_key_generation keygen 2,830,452 2,830,452 +0.0%
libsignal_benchmark::session_optimization_group::bench_decrypt_with_previous_session previous_session:setup_with_archived_sessions() 46,566 46,566 +0.0%
libsignal_benchmark::session_optimization_group::bench_out_of_order_decryption out_of_order:setup_out_of_order_messages() 5,197,012 5,197,012 +0.0%
libsignal_benchmark::session_optimization_group::bench_promote_matching_session promote:setup_promote_matching_session() 360,648 360,648 +0.0%
libsignal_benchmark::session_optimization_group::bench_message_key_eviction eviction:setup_message_key_eviction() 14,255,917 14,255,917 +0.0%

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@wacore/benches/send_receive_benchmark.rs`:
- Around line 736-740: bench_group_send's warm-path calls run_group_send which
invokes prepare_group_stanza with all_devices_for_phash set to None, causing
distribution_list to remain None and no phash to be computed; change the call in
run_group_send (the invocation that passes skdm_target_devices and
all_devices_for_phash) to pass the resolved distribution set (the variable
holding the selected device list/distribution_list) into the
all_devices_for_phash parameter instead of None so prepare_group_stanza can
compute and emit the group phash (this aligns with build_group_phash_set which
expects the full device set to append the sender and filter hosted devices).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f931f8dd-0435-4bf5-ba8f-dc76ad659106

📥 Commits

Reviewing files that changed from the base of the PR and between 483a896 and 4383cb7.

📒 Files selected for processing (4)
  • src/send.rs
  • wacore/benches/send_receive_benchmark.rs
  • wacore/src/messages.rs
  • wacore/src/send.rs

Comment thread wacore/benches/send_receive_benchmark.rs
The warm group-send benches (setup_group_send_*, force_skdm=false) called
prepare_group_stanza with all_devices_for_phash=None, so distribution_list
stayed None and no phash was computed. Real warm sends now always compute a
phash (full device set + self), so the bench under-measured that cost.

Pass the participant device set on the warm path (mirroring the real caller,
which supplies the full set when not distributing); the cold/force_skdm path
keeps None and resolves the set itself.
@jlucaso1
jlucaso1 merged commit 1613e56 into main Jun 1, 2026
11 checks passed
@jlucaso1
jlucaso1 deleted the fix/group-skdm-phash-base64-and-haskey branch June 1, 2026 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant